Fix: Orphaned Partial Ask Messages // Message Guard #3514
Conversation
catrielmuller
requested review from
RSO,
chrarnoldus,
Copilot and
kevinvandijk
|
There was a problem hiding this comment.
Pull Request Overview
This PR implements a message insertion guard mechanism to prevent race conditions when checkpoint messages arrive during partial message updates in the Task system. The guard ensures messages are inserted sequentially, preventing message history order corruption.
Key changes:
- Added
MessageInsertionGuardclass with acquire/release locking and wait-for-clearance mechanism - Integrated the guard into
Task.addToClineMessages()to serialize message insertions - Added comprehensive test suite covering basic locking, concurrent access, error handling, and realistic checkpoint scenarios
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/core/kilocode/task/message-utils.ts | Adds MessageInsertionGuard class to handle message insertion synchronization |
| src/core/task/Task.ts | Integrates guard into addToClineMessages() method and imports the new guard class |
| src/core/task/tests/message-insertion-guard.spec.ts | Comprehensive test suite for the message insertion guard functionality |
Comments suppressed due to low confidence (1)
src/core/task/Task.ts:694
- The
updateClineMessagemethod modifies existing messages and callspostMessageToWebviewwithout acquiring the insertion guard. If this executes concurrently withaddToClineMessages, it could lead to inconsistent state updates or ordering issues in the webview. Consider protecting message updates with the guard as well.
private async updateClineMessage(message: ClineMessage) {
const provider = this.providerRef.deref()
await provider?.postMessageToWebview({ type: "messageUpdated", clineMessage: message })
this.emit(RooCodeEventName.Message, { action: "updated", message })
catrielmuller
force-pushed
the
catrielmuller/fix-partial-ask-msg-reviewed
branch
from
8b63efc to
01fdc8c
Compare
Improved version of: #3468
Context
Fixed a race condition where checkpoint messages (checkpoint_saved) could arrive during partial message updates, causing the message history to break and leaving partial messages orphaned with partial=true state.
Root Cause:
The checkpoint service runs git operations asynchronously in the background. When a checkpoint completes, it calls task.say("checkpoint_saved", ...) through an event handler, which can execute while Task.ask() or Task.say() are still updating partial messages. This resulted in checkpoint messages being inserted between a partial message's start and completion, breaking the message chain.
Previous Workaround:
The findPartialAskMessage() and findPartialSayMessage() functions were added to search backwards through messages to locate orphaned partials, but this was a symptom of the underlying race condition.
Implementation
Implemented a Message Insertion Guard pattern that serializes all message insertions without blocking checkpoint git operations.
Program Flow
Message Insertion Sequence (with guard):
Checkpoint Flow (unchanged, still async):
How to Test
Prerequisites
Test Scenario 1: Verify No Orphaned Partial Messages
Expected: Message history remains in correct order with no partial=true messages left behind
How to verify:
Test Scenario 2: Verify Checkpoint Performance
Test Scenario 3: Run Automated Tests